Add "remove and withdraw all" option to order removal (#2024)#2722
Add "remove and withdraw all" option to order removal (#2024)#2722thedavidmeister wants to merge 8 commits into
Conversation
Issue #2024 asks to offer withdrawing an order's vault balances at the same time as removing the order, without making withdrawal automatic (orders can share vaults). In OrderDetail.svelte the standalone Remove button becomes a dropdown (when the new optional onRemoveAndWithdrawAll callback is provided) offering: - "Remove" (unchanged behaviour), and - "Remove and withdraw all vaults". The webapp wires the new option to handleRemoveOrderAndWithdrawAll, which mirrors handleVaultsWithdrawAll but builds a single multicall([removeOrder, withdraw]) so removal and withdrawal execute atomically. Withdraw calldata is only included when the order actually has withdrawable (positive-balance) vaults, so an empty order is still removed. When onRemoveAndWithdrawAll is not supplied, OrderDetail renders the original single Remove button, so existing callers are unaffected. Tests: - ui-components OrderDetail: new tests assert the dropdown replaces the bare button, that each item invokes the correct callback (Remove -> onRemove with the order; Remove-and-withdraw -> onRemoveAndWithdrawAll with the order and its vaultsList), and that the dropdown is hidden for inactive orders. - webapp handleRemoveOrderAndWithdrawAll: new tests assert the exact multicall calldata bytes and decode it to prove the flat [removeOrder, withdraw] order, that an empty order produces a remove-only multicall (and never requests withdraw calldata), the combined modal title, the remove-order transaction on confirm, and that remove/withdraw/lookup errors toast and abort. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe order detail UI now offers a remove dropdown with an optional withdraw-all action. A new webapp service builds the combined transaction, opens a confirmation modal, and the order route wires the handler into the page. ChangesRemove-and-withdraw order flow
Sequence Diagram(s)sequenceDiagram
participant OrderDetail.svelte
participant handleRemoveOrderAndWithdrawAll
participant order
participant vaultsList
participant openTransactionConfirmationModal
participant manager.createRemoveOrderTransaction
OrderDetail.svelte->>handleRemoveOrderAndWithdrawAll: onRemoveAndWithdrawAll(raindexClient, order, vaultsList)
handleRemoveOrderAndWithdrawAll->>order: getRemoveCalldata()
handleRemoveOrderAndWithdrawAll->>vaultsList: getWithdrawableVaults()
handleRemoveOrderAndWithdrawAll->>vaultsList: getWithdrawCalldata()
handleRemoveOrderAndWithdrawAll->>openTransactionConfirmationModal: encoded multicall calldata
openTransactionConfirmationModal->>manager.createRemoveOrderTransaction: txHash and order metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/ui-components/src/__tests__/OrderDetail.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
packages/ui-components/src/lib/components/detail/OrderDetail.svelteOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
packages/webapp/src/__tests__/handleRemoveOrderAndWithdrawAll.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
UI screenshot: "remove and withdraw all" option on order removal |
Restate two added test comments as present-tense facts about what the test asserts now, dropping before/after and hypothetical-mutation framing. Comment-only; no code or logic changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Union: take main's LocalDbSyncGate wrapper around OrderDetail AND keep this branch's onRemoveAndWithdrawAll prop (not present on main yet). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-06-13-issue-2024
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui-components/src/lib/components/detail/OrderDetail.svelte`:
- Around line 159-175: The OrderDetail menu is showing “Remove and withdraw all
vaults” whenever onRemoveAndWithdrawAll exists, even for zero-balance
vaultsList. Update the conditional in OrderDetail.svelte around the
Dropdown/DropdownItem branch to also check that the order has withdrawable
balance before rendering the extra action, using the existing data.vaultsList
shape. Keep the plain Remove DropdownItem available for empty vaults, and if
needed align the label/disabled state so the UI does not imply withdrawable
funds when there are none.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 927d6d2b-0738-4ad7-b081-c7143b1f5977
📒 Files selected for processing (5)
packages/ui-components/src/__tests__/OrderDetail.test.tspackages/ui-components/src/lib/components/detail/OrderDetail.sveltepackages/webapp/src/__tests__/handleRemoveOrderAndWithdrawAll.test.tspackages/webapp/src/lib/services/handleRemoveOrderAndWithdrawAll.tspackages/webapp/src/routes/orders/[chainId]-[raindex]-[orderHash]/+page.svelte
| {#if onRemoveAndWithdrawAll} | ||
| <Button | ||
| id="remove-order-menu" | ||
| data-testid="remove-order-menu" | ||
| aria-label="Remove order"> | ||
| Remove | ||
| <ChevronDownOutline size="sm" class="ml-2" /> | ||
| </Button> | ||
| <Dropdown placement="bottom-end" triggeredBy="#remove-order-menu"> | ||
| <DropdownItem | ||
| on:click={() => onRemove(raindexClient, data)} | ||
| data-testid="remove-button">Remove</DropdownItem | ||
| > | ||
| <DropdownItem | ||
| on:click={() => onRemoveAndWithdrawAll(raindexClient, data, data.vaultsList)} | ||
| data-testid="remove-and-withdraw-all-button" | ||
| >Remove and withdraw all vaults</DropdownItem |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Only show the withdraw-all action when the order actually has withdrawable balance.
This branch only checks whether onRemoveAndWithdrawAll exists, so a zero-balance vaultsList still advertises "Remove and withdraw all vaults" even though the downstream handler degrades to a plain remove. The new OrderDetail.test.ts fixture uses that exact all-zero shape, so this misleading path is already being asserted. Gate the extra item on a withdrawable-balance check (or disable/rename it) and keep the plain remove action for empty vaults.
💡 Suggested change
+ const hasWithdrawableVaults = (vaultsList: RaindexVaultsList) =>
+ vaultsList.items.some((vault) => vault.balance > 0n);
...
- {`#if` onRemoveAndWithdrawAll}
+ {`#if` onRemoveAndWithdrawAll && hasWithdrawableVaults(data.vaultsList)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {#if onRemoveAndWithdrawAll} | |
| <Button | |
| id="remove-order-menu" | |
| data-testid="remove-order-menu" | |
| aria-label="Remove order"> | |
| Remove | |
| <ChevronDownOutline size="sm" class="ml-2" /> | |
| </Button> | |
| <Dropdown placement="bottom-end" triggeredBy="#remove-order-menu"> | |
| <DropdownItem | |
| on:click={() => onRemove(raindexClient, data)} | |
| data-testid="remove-button">Remove</DropdownItem | |
| > | |
| <DropdownItem | |
| on:click={() => onRemoveAndWithdrawAll(raindexClient, data, data.vaultsList)} | |
| data-testid="remove-and-withdraw-all-button" | |
| >Remove and withdraw all vaults</DropdownItem | |
| const hasWithdrawableVaults = (vaultsList: RaindexVaultsList) => | |
| vaultsList.items.some((vault) => vault.balance > 0n); | |
| {`#if` onRemoveAndWithdrawAll && hasWithdrawableVaults(data.vaultsList)} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui-components/src/lib/components/detail/OrderDetail.svelte` around
lines 159 - 175, The OrderDetail menu is showing “Remove and withdraw all
vaults” whenever onRemoveAndWithdrawAll exists, even for zero-balance
vaultsList. Update the conditional in OrderDetail.svelte around the
Dropdown/DropdownItem branch to also check that the order has withdrawable
balance before rendering the extra action, using the existing data.vaultsList
shape. Keep the plain Remove DropdownItem available for empty vaults, and if
needed align the label/disabled state so the UI does not imply withdrawable
funds when there are none.
1 participant
Fixes #2024
What the issue asked
Allow withdrawing an order's vault balances at the same time as removing the
order. Per the issue, withdrawal must not be automatic (orders can share
vaults), so the Remove button should become a context menu offering "remove"
and "remove and withdraw all vaults".
What changed (TS/webapp only — no contract/bytecode changes)
OrderDetail.svelte— when the new optionalonRemoveAndWithdrawAllcallback is provided, the standalone Remove button is replaced by a dropdown
with two items: Remove (unchanged behaviour) and Remove and withdraw all
vaults. When the callback is not supplied the original single Remove button
renders, so existing callers are unaffected.
handleRemoveOrderAndWithdrawAll.ts(new webapp service) — mirrorshandleVaultsWithdrawAll, but generates a singlemulticall([removeOrder, withdraw])so the order removal and the vaultwithdrawals execute atomically in one transaction. The withdraw calldata comes
from
vaultsList.getWithdrawCalldata()and is only appended when the orderactually has withdrawable (positive-balance) vaults — an order whose vaults
are all empty is still removed. The multicall is built with viem's
encodeFunctionDataagainst the Raindexmulticall(bytes[])ABI (selector0xac9650d8, inherited from OpenZeppelinMulticall).+page.svelte— wires the newonRemoveAndWithdrawAllprop to the new service.
Tests (all discriminating)
handleRemoveOrderAndWithdrawAll.test.ts(7 tests):[removeOrder, withdraw]and decodes it to prove the flat order (a swapped order or a missing withdraw
changes the bytes — verified by mutation);
multicall and never even calls
getWithdrawCalldata;confirm with the right args, and that remove-calldata / withdraw-calldata /
withdrawable-lookup errors each toast and abort without opening the modal.
OrderDetail.test.ts(4 new tests): the dropdown replaces the bare buttonwhen
onRemoveAndWithdrawAllis set; the Remove item callsonRemove(and notthe other); the Remove-and-withdraw item calls
onRemoveAndWithdrawAllwiththe order and its
vaultsList(and notonRemove) — verified by mutation; andthe dropdown is hidden for inactive orders.
Verification
npm run check: ui-components 0 errors; webapp 2 pre-existing errors only(
PUBLIC_WALLETCONNECT_PROJECT_IDmissing env var, in untouched files).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests